Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid redefinition of _bit_scan_{forward,reverse} macros #248

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Avoid redefinition of _bit_scan_{forward,reverse} macros #248

merged 1 commit into from
Dec 4, 2020

Conversation

amadio
Copy link
Collaborator

@amadio amadio commented Mar 6, 2020

Fixes #247.

Axel-Naumann
Axel-Naumann reviewed Mar 6, 2020
View reviewed changes
Vc/common/bitscanintrinsics.h Outdated
@@ -32,7 +32,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# if Vc_GCC >= 0x40500
// GCC 4.5.0 introduced _bit_scan_forward / _bit_scan_reverse
# include <x86intrin.h>
# else
# elif !defined(_bit_scan_forward)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apple clang has /Library/Developer/CommandLineTools//usr/lib/clang/11.0.0/include/x86intrin.h. Shouldn't we use that instead of entering the #elif branch here? I.e. is #if Vc_GCC >= 0x40500 really correct, or should it be #if Vc_GCC >= 0x40500 || defined(Vc_CLANG) || defined(Vc_APPLECLANG)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, it's the other option. I just don't know which versions of clang have or don't have the macro. Maybe generically defining it if it's not defined is better than both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that even if the header exists, sometimes the macro is not defined. So we can include the header and if the macro is not defined, we define it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but what happens if x86intrin.h defines the macro but wasn't included yet? This header would still define the macro and a subsequent #include <x86intrin.h> leads to an error?
I think the right fix is to unconditionally include x86intrin.h for GCC, Clang and AppleClang before testing !defined(_bit_scan_forward).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. I will update the change. There were a couple of failures in Travis probably due to that. BTW, do you plan to make a new patch release of 1.4 anytime soon?

mattkretz
mattkretz requested changes Mar 6, 2020
View reviewed changes
Copy link
Member

@mattkretz mattkretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! See my comment inline

Vc/common/bitscanintrinsics.h Outdated
@@ -32,7 +32,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# if Vc_GCC >= 0x40500
// GCC 4.5.0 introduced _bit_scan_forward / _bit_scan_reverse
# include <x86intrin.h>
# else
# elif !defined(_bit_scan_forward)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but what happens if x86intrin.h defines the macro but wasn't included yet? This header would still define the macro and a subsequent #include <x86intrin.h> leads to an error?
I think the right fix is to unconditionally include x86intrin.h for GCC, Clang and AppleClang before testing !defined(_bit_scan_forward).

@amadio
Copy link
Collaborator Author

amadio commented Mar 9, 2020

How's this version? I tried to run tests, but ran into

[ 10%] Building CXX object tests/CMakeFiles/logarithm_avx2.dir/logarithm.cpp.o
In file included from /Users/sftnight/amadio/Vc/tests/logarithm.cpp:28:
/Users/sftnight/amadio/Vc/tests/unittest.h:30:10: fatal error: 'virtest/vir/test.h' file not found
#include "virtest/vir/test.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

which I don't know how to fix. What's providing this header? I guess it's an external dependency, but CMake configured without errors, so I'm confused.

@amadio
Copy link
Collaborator Author

amadio commented Mar 10, 2020

Hi @mattkretz, I cannot see what the error was, so I don't know if it's related to my changes. Can you please let me know?

@Axel-Naumann
Copy link
Member

Ping, @mattkretz please! :-)

@Axel-Naumann
Copy link
Member

@amadio Google says it's https://github.com/mattkretz/virtest - could you try with that, please?

@hahnjo
Copy link
Contributor

hahnjo commented Dec 3, 2020

How's this version? I tried to run tests, but ran into

[ 10%] Building CXX object tests/CMakeFiles/logarithm_avx2.dir/logarithm.cpp.o
In file included from /Users/sftnight/amadio/Vc/tests/logarithm.cpp:28:
/Users/sftnight/amadio/Vc/tests/unittest.h:30:10: fatal error: 'virtest/vir/test.h' file not found
#include "virtest/vir/test.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

which I don't know how to fix. What's providing this header? I guess it's an external dependency, but CMake configured without errors, so I'm confused.

The header comes from a submodule (of the repo that Axel posted), the README mentions git submodule update --init.

After doing this, I was able to run the tests and all pass with this change (GCC 8.3.1 and Clang 9.0.1 from CentOS 8) while making the build with Clang free of warnings. Can we merge this one? @mattkretz

@agheata
Copy link
Collaborator

agheata commented Dec 3, 2020

Can we merge this now?

@mattkretz
Copy link
Member

Looking at it now. Probably yes. :-)

@mattkretz
Copy link
Member

Can we use C++20? 😜
The bsr inline asm for clang is scary. I'm sure we can use a __builtin nowadays. The PR seems to be a strict improvement, but I'd rather rewrite this header altogether. I'll merge it for now and might push a cleanup later.

@mattkretz mattkretz merged commit 383458a into VcDevel:1.4 Dec 4, 2020
@bernhardmgruber bernhardmgruber added this to the Vc 1.4.2 milestone Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattkretz mattkretz mattkretz requested changes

@Axel-Naumann Axel-Naumann Axel-Naumann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Vc 1.4.2
Development

Successfully merging this pull request may close these issues.

Xcode 11.4 beta2: _bit_scan_forward redefinition
6 participants
@amadio @Axel-Naumann @hahnjo @agheata @mattkretz @bernhardmgruber